A11y: enable rule jsx-a11y/anchor-is-valid #56690
Conversation
eledobleefe
added
type/accessibility
| @@ -87,14 +87,14 @@ export const InputWithAutoFocus = () => { | |||
| {inputComponents.map((InputComponent: any, i: number) => ( | |||
| <InputComponent initialValue="test" key={i} /> | |||
| ))} | |||
| <a | |||
| <button | |||
There was a problem hiding this comment.
We'd be careful when changing anything to button as it has submit type by default. Normally its not a big deal, but when used in forms it will trigger a default submit behavior. Therefore I think we'd add type='button' to all the buttons by default (our Button component already has that).
There was a problem hiding this comment.
Thanks for your comment @Clarity-89 😊 I will add type='button'.
There was a problem hiding this comment.
generally looks great 🙌
added a couple of minor nits and as previously mentioned we need to:
- pull out the clearing of these button styles into some reusable/importable function that everywhere can import
- make sure we add
type="button"anywhere we're using the native<button>element to prevent form submission 🙂
public/app/plugins/datasource/prometheus/components/PromQueryField.tsx
Outdated
Show resolved
Hide resolved
public/app/plugins/datasource/cloudwatch/components/LogsQueryField.tsx
Outdated
Show resolved
Hide resolved
public/app/plugins/panel/gettingstarted/components/TutorialCard.tsx
Outdated
Show resolved
Hide resolved
| <button | ||
| role="checkbox" | ||
| aria-checked={option.selected} | ||
| className={`${highlightClass} ${noStyledButton}`} |
There was a problem hiding this comment.
think we should use classnames here instead of interpolated strings
There was a problem hiding this comment.
Hi @ashharrison90. In this case, highlightClasswas already there and its value is set according to a condition so I think this shouldn't be changed. Regarding noStyledButton, we could either let it be or change it to use classnames. What do you think is better?
There was a problem hiding this comment.
yeah i think that's fine, i'm saying use the classnames package to apply multiple classes instead of using string interpolation
e.g. classNames={classnames(highlightClass, noStyledButton)}
There was a problem hiding this comment.
Ah, ok. I didn't know about that 🤔 Thank you @ashharrison90!!!
There was a problem hiding this comment.
Aren't we using cx from emotion for this kind of stuff?
There was a problem hiding this comment.
@Clarity-89 last thing i remember is we decided we can use either
cx performance is much worse than classnames: #38698
There was a problem hiding this comment.
Ah, makes sense. Didn't know cx had such a poor performance.
There was a problem hiding this comment.
the only thing we have to be careful of is when composing multiple emotion css classes... tbh if the cx performance wasn't like 4-5 times worse we should definitely be using cx everywhere 😅
eledobleefe
requested review from
joshhunt,
JoaoSilvaGrafana,
leventebalogh,
codeincarnate and
supilee
and removed request for
a team
| @@ -121,20 +123,30 @@ export function FilterSection({ | |||
| return ( | |||
| <InlineFormLabel key={idx} width="auto" data-testid={testIds.list + idx}> | |||
| {fil.tagk} = {fil.type}({fil.filter}), groupBy = {'' + fil.groupBy} | |||
| <a onClick={() => editFilter(fil, idx)}> | |||
| <button type="button" className={clearButtonStyles(theme)} onClick={() => editFilter(fil, idx)}> | |||
There was a problem hiding this comment.
I think we could've done smth like const buttonStyles = useStyles2(clearButtonStyles) at the top and use that throughout the component instead of calling clearButtonStyles in multiple places.
| @@ -23,6 +23,8 @@ export function TagSection({ | |||
| suggestTagValues, | |||
| tsdbVersion, | |||
| }: TagSectionProps) { | |||
| const theme = useTheme2(); | |||
|
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/38126 |
|
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/38338 |
|
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/38475 |
|
Hi @grafana/observability-metrics! |
|
Can verify that change to prometheus hint link works fine and is keyboard navigable, can be triggered via or key presses while focused.
@bohandley Can you take a look at the openTSDB changes when you have a chance? |
|
I think this breaks padding in option selector. Before:
After:
This is seemingly because
Is this intentional? It doesn't seem like it is, but maybe I'm missing something. $ git show e402a8f27d9faabac8181c847b06014ae29af0d5 -- packages/grafana-ui/src/components/Button/Button.tsx
commit e402a8f27d9faabac8181c847b06014ae29af0d5
Author: Laura Fernández <laura.fernandez@grafana.com>
Date: Fri Oct 21 09:13:32 2022 +0200
A11y: enable rule jsx-a11y/anchor-is-valid (#56690)
diff --git a/packages/grafana-ui/src/components/Button/Button.tsx b/packages/grafana-ui/src/components/Button/Button.tsx
index 4813543f9c..ae3a903295 100644
--- a/packages/grafana-ui/src/components/Button/Button.tsx
+++ b/packages/grafana-ui/src/components/Button/Button.tsx
@@ -302,3 +302,12 @@ export function getPropertiesForVariant(theme: GrafanaTheme2, variant: ButtonVar
return getButtonVariantStyles(theme, theme.colors.primary, fill);
}
}
+
+export const clearButtonStyles = (theme: GrafanaTheme2) => {
+ return css`
+ background: transparent;
+ color: ${theme.colors.text.primary};
+ border: none;
+ padding: 0;
+ `;
+}; |
What this PR does / why we need it:
Enable rule
jsx-a11y/anchor-is-validand fix current errors.Which issue(s) this PR fixes:
Fixes #55834
Special notes for your reviewer: